Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check notification service before enable #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FlowBreeze
Copy link

fix issue: #42

@MichaelAquilina
Copy link
Owner

@FlowBreeze thanks for the PR :) you need to install dbus-send in circleci OR mock it in order to get the tests to pass.

Let me know if you need a hand with this. Happy to help

@FlowBreeze
Copy link
Author

@MichaelAquilina I'm Sorry, I never use circleci before so I just rewrite my code and add dbus-send check.
If you need to configure circleci, you may do it by yourself.

# Check dbus-send command exists
if ! type dbus-send > /dev/null; then
# step to default behaviour if dbus-send not exits
print "yes"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt this be no 🤔 ?

@FlowBreeze
Copy link
Author

FlowBreeze commented Apr 29, 2023

@MichaelAquilina
yes in here is unclear, so i change it to 'unknown'

here is my consideration:
dbus-send is a freedesktop buildin tool,
So I think it is rarely occurred an system has notify-send but not dbus-send until i met an error from circle ci

  • if system has dus-send and doesn't hava a 'org.freedesktop.Notifications' consumer,then disable notify
  • if system has notify-send but not dus-send,then enable notify service to keep the backward capability (and pass circleci check), although i think it's rarely happens

And It's ok to change my code as you like

@FlowBreeze FlowBreeze force-pushed the fix-notify-send-timeout branch from fac4435 to 92d6926 Compare September 11, 2023 08:50
niraami added a commit to niraami/zsh-auto-notify that referenced this pull request Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants